-
Notifications
You must be signed in to change notification settings - Fork 14
Implement dwell timers #465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
* Initialize on geofence event * Add tests to check priority insertion, instant flush, and init effect * guard against empty companyIds form malformed geofences * Reject geofence events if they have a mismatching API key to the one in store
…nd, awake on geo event
… more clear separation
Sources/KlaviyoLocation/KlaviyoLocationManager+CLLocationManagerDelegate.swift
Show resolved
Hide resolved
Sources/KlaviyoLocation/KlaviyoLocationManager+CLLocationManagerDelegate.swift
Show resolved
Hide resolved
Sources/KlaviyoLocation/KlaviyoLocationManager+CLLocationManagerDelegate.swift
Show resolved
Hide resolved
Firstly, thanks for the detailed notes on the PR. Super useful for reviewing. If we should fire the dwell event in the future seemed to me like a product question so checking in with PMs. |
ab1470
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, although it seems like maybe we're not going to merge this?
| /// Optional duration for this geofence to record a dwell event | ||
| var duration: Int? { | ||
| let components = id.split(separator: ":", omittingEmptySubsequences: false) | ||
| guard components.count == 4, components[0] == "_k" else { return nil } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't we have an isKlaviyo... method somewhere to check if something starts with _k (and therefore is a Klaviyo event/notification)?
| geofenceDwellSettings.removeAll() | ||
| for geofence in geofences { | ||
| geofenceDwellSettings[geofence.locationId] = geofence.duration | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code will remove all geofence dwell settings, including those that may not have been included in the Set that the caller supplied when they called this method. Is that okay?
| if #available(iOS 14.0, *) { | ||
| Logger.geoservices.info("🕐 Fired expired dwell event for region \(geofenceId) (expired while app was terminated)") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should go inside the Task, below the full await clause. Technically the current order is incorrect, as you're dispatching an async task that may or may not complete, and then you're immediately logging the event before waiting for the task to finish
| // Check if timer expired (elapsed >= duration) | ||
| if age >= TimeInterval(timerData.duration) { | ||
| timersToRemove.append(geofenceId) | ||
| if currentTime - timerData.startTime >= TimeInterval(timerData.duration) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you create a private helper function (or computed property) to abstract this logic and make it more readable? Something like:
if timerData.isExpired {
...
}|
Closing for now as we have decided due to iOS's limitations of not natively supporting dwell and consequently requiring intrusive/overly complex workarounds to even provide a best effort level of support, we will not support it for this milestone. We may revisit later and/or include some of these model changes, so we could flexibly support it in the future and be backwards compatible. If we decide to do those model changes/more incremental support, we will do those in new PRs. |
Description
Easiest to review commit by commit but basically --
iOS does not have native support for dwell events. Instead, we can only go off of didEnter/didExit events and tracking potential dwell events via timers and do our best to catch if they have expired or not.
These timers are stored in memory.
Tricky part is, timers only run while the app is in the foreground. So, here is where we start doing a best-effort to catch expired dwell timers by also persisting their time stamps to UserDefaults
Due Diligence
Release/Versioning Considerations
PatchContains internal changes or backwards-compatible bug fixes.MinorContains changes to the public API.MajorContains breaking changes.Changelog / Code Overview
Test Plan
I used a mocked response for GET
client/geofencesin this commit to test getting a geofence response with durations. I then tested all of the following scenarios and some other variants:Related Issues/Tickets
https://klaviyo.atlassian.net/browse/CHNL-28037